feat: add i18n support for 5 languages (en, es, fr, de, zh) #593
Conversation
RIVALHIDE
requested review from
Anshgrover23,
Suyashd999 and
mikejmorgan-ai
as code owners
📝 WalkthroughWalkthroughAdds a complete i18n subsystem (translator, catalogs, detector, formatter, persisted LanguageConfig), integrates language selection and auto-detection into the CLI (global flag and config subcommands), ships five locale catalogs, updates packaging and docs, expands tests, and relaxes some integration assertions for localized outputs. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI (user)"
participant Config as "LanguageConfig\n(~/.cortex/preferences.yaml)"
participant Translator as "Translator\n(catalogs)"
participant OS as "OS env\n(LANG/LC_*/LANGUAGE)"
participant FS as "Filesystem"
rect rgba(200,220,255,0.5)
CLI->>Config: request current language (startup / config show)
Config-->>CLI: return language (env override / saved / detected)
end
rect rgba(200,255,200,0.5)
CLI->>Translator: set language (flag / config set)
Translator->>Translator: load/reload catalogs (en + target)
Translator-->>CLI: translations available
end
rect rgba(255,230,200,0.5)
Translator->>FS: read locales/{lang}.yaml
Config->>FS: read/write preferences.yaml (atomic + lock)
end
rect rgba(240,200,255,0.5)
OS-->>Config: provide detected locale (detect_os_language)
Config-->>Translator: fallback language if no saved/env
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @RIVALHIDE, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Cortex CLI by integrating full internationalization capabilities. It allows the CLI to adapt to the user's language preference, providing a localized experience across all messages and data formatting. This change broadens the accessibility and usability of Cortex for a global user base, making it more intuitive and user-friendly regardless of their native language. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive and well-structured internationalization (i18n) framework, which is a fantastic addition to the CLI. The implementation is thorough, covering translation, configuration, auto-detection, formatting, and testing. The new documentation and test suite are particularly impressive. My review focuses on a few areas for improvement to enhance maintainability and performance:
- Resolving a circular dependency within the
i18nmodule to improve the overall design and remove code duplication. - Optimizing a language name resolution function for better performance.
- Suggesting a more direct use of the
babellibrary for formatting to simplify the code and improve scalability.
Overall, this is a very strong contribution that significantly enhances the user experience.
I am having trouble creating individual review comments. Click here to see my feedback.
cortex/i18n/config.py (20-21)
This SUPPORTED_LANGUAGES constant is a duplicate of the one defined in cortex/i18n/translator.py. This duplication can lead to maintenance issues if the two get out of sync. It appears to be a workaround for a circular dependency between config.py and translator.py.
To resolve this, I recommend creating a single source of truth in a new file, for example cortex/i18n/constants.py. Both modules could then import from this new file, which would break the dependency cycle and eliminate the duplicated constant.
Example for cortex/i18n/constants.py:
SUPPORTED_LANGUAGES: dict[str, dict[str, str]] = {
"en": {"name": "English", "native": "English"},
# ... other languages
}
DEFAULT_LANGUAGE = "en"This change would significantly improve the design and robustness of the i18n module.
cortex/cli.py (978-981)
This local import (from cortex.i18n.translator import reset_translator) is used here and in other places to work around a circular dependency. While this pattern works, it's often a sign of a design issue that could be improved.
Resolving the underlying circular dependency (as suggested in another comment regarding cortex/i18n/config.py) would allow this import to be moved to the top of the file, improving code readability and structure.
lang_config.clear_language()
reset_translator()
cortex/cli.py (2140-2178)
The _resolve_language_name function rebuilds the name_to_code lookup dictionary on every call. Since the list of supported languages is static, this dictionary can be created once and cached for better performance.
You can achieve this by attaching the lookup table to the function object itself, so it's only computed on the first call. This avoids polluting the module's global namespace while still being efficient.
def _resolve_language_name(name: str) -> str | None:
"""
Resolve a language name or code to a supported language code.
Accepts:
- Language codes: en, es, fr, de, zh
- English names: English, Spanish, French, German, Chinese
- Native names: Español, Français, Deutsch, 中文
Args:
name: Language name or code (case-insensitive for Latin scripts)
Returns:
Language code if found, None otherwise
"""
if not hasattr(_resolve_language_name, "_lookup"):
lookup = {}
for code, info in SUPPORTED_LANGUAGES.items():
lookup[code] = code
lookup[info["name"].lower()] = code
lookup[info["native"].lower()] = code
lookup[info["native"]] = code # For case-sensitive names like 中文
_resolve_language_name._lookup = lookup
stripped_name = name.strip()
# Check lowercased version first, then original for case-sensitive names
return _resolve_language_name._lookup.get(stripped_name.lower()) or _resolve_language_name._lookup.get(stripped_name)cortex/i18n/formatter.py (148-153)
The LocaleFormatter class provides a custom implementation for formatting dates, numbers, and other data types. While functional, this approach is difficult to maintain and scale, as it requires adding large configuration blocks to LOCALE_CONFIGS for each new language.
The pull request description mentions using Babel. I recommend refactoring this class to use the babel library's formatting functions directly. This would leverage its extensive locale data, simplify the code, and make it more robust.
For example, format_date could be simplified to:
from babel.dates import format_date as babel_format_date
def format_date(self, dt: datetime) -> str:
"""Formats a date using babel."""
return babel_format_date(dt, locale=self.language)Similar refactoring can be applied to format_number, format_datetime, and format_time_ago, which would remove the need for the large LOCALE_CONFIGS dictionary.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cortex/i18n/config.py`:
- Around line 54-60: The TOCTOU arises because the existence check for
self.preferences_file happens before acquiring self._file_lock; move the
exists() check inside the critical section so the lock covers both the check and
the file read in _load_preferences (acquire self._file_lock, then if
self.preferences_file.exists(): open/read with yaml.safe_load and return or {}),
and keep the existing exception handling (yaml.YAMLError, OSError) to handle
race-related failures.
In `@cortex/i18n/formatter.py`:
- Around line 358-370: get_formatter currently caches a LocaleFormatter once and
never updates it when the app language changes; modify get_formatter so that
after importing and calling get_language() it compares that language to the
existing _formatter's language and, if different, either set _formatter.language
to the new value (if LocaleFormatter exposes a setter) or re-create _formatter =
LocaleFormatter(language=new_lang); ensure this logic lives inside get_formatter
so every access stays in sync and also handle the case where _formatter is None
by creating it as before.
🧹 Nitpick comments (8)
docs/i18n-guide.md (1)
188-195: Add language specifier to fenced code block.The code block showing the directory structure is missing a language specifier, which triggers a markdown lint warning (MD040). Use
textorplaintextfor non-code content.🛠️ Suggested fix
-``` +```text cortex/i18n/locales/ ├── en.yaml # English (source language) ├── es.yaml # Spanish ├── fr.yaml # French ├── de.yaml # German └── zh.yaml # Chinese</details> </blockquote></details> <details> <summary>tests/test_i18n.py (3)</summary><blockquote> `649-659`: **Path construction may be fragile across different test execution contexts.** The path construction at line 655 assumes a specific relative directory structure from the test file location. This could break if tests are run from different working directories or if the project structure changes. <details> <summary>♻️ Suggested improvement using package resource discovery</summary> ```diff def test_all_supported_languages_have_catalogs(self): """Test that all supported languages have message catalogs.""" - from pathlib import Path - from cortex.i18n.translator import SUPPORTED_LANGUAGES + import importlib.resources - locales_dir = Path(__file__).parent.parent / "cortex" / "i18n" / "locales" + # Use importlib.resources for reliable package resource discovery + try: + # Python 3.9+ + locales_ref = importlib.resources.files("cortex.i18n") / "locales" + except AttributeError: + # Fallback for older Python + from pathlib import Path + locales_dir = Path(__file__).parent.parent / "cortex" / "i18n" / "locales" + else: + locales_dir = locales_ref for lang_code in SUPPORTED_LANGUAGES: - catalog_path = locales_dir / f"{lang_code}.yaml" - self.assertTrue(catalog_path.exists(), f"Missing catalog for language: {lang_code}") + # Check if catalog exists + catalog_name = f"{lang_code}.yaml" + if hasattr(locales_dir, 'joinpath'): + catalog_path = locales_dir.joinpath(catalog_name) + exists = catalog_path.is_file() if hasattr(catalog_path, 'is_file') else True + else: + catalog_path = locales_dir / catalog_name + exists = catalog_path.exists() + self.assertTrue(exists, f"Missing catalog for language: {lang_code}")Alternatively, you could verify catalog existence by attempting to load them through the Translator class, which already handles resource discovery internally.
20-21: Unused import:yaml.The
yamlmodule is imported but never used in the test file. This was likely intended for direct YAML parsing but the tests use the Translator class instead.🧹 Remove unused import
from unittest.mock import MagicMock, patch -import yaml - class TestTranslator(unittest.TestCase):
18-18: Unused import:MagicMock.
MagicMockis imported but not used in this test file. Onlypatchis used fromunittest.mock.🧹 Remove unused import
-from unittest.mock import MagicMock, patch +from unittest.mock import patchcortex/i18n/config.py (1)
19-21: Consider renaming to avoid confusion withtranslator.SUPPORTED_LANGUAGES.The comment acknowledges the circular import concern, but using the same name for different types (set here vs dict in translator.py) can confuse maintainers. Consider renaming to
SUPPORTED_LANGUAGE_CODESto clarify it's just for membership checking.cortex/cli.py (1)
1164-1177: Consider narrowing the exception type.The catch-all
Exceptionon line 1175 masks the specific error. Sinceset_languageraisesValueErrorand_save_preferencesraisesRuntimeError, catching these specifically would provide better error handling.♻️ Proposed refinement
try: lang_config.set_language(code) # Reset the global translator to pick up the new language from cortex.i18n.translator import reset_translator reset_translator() set_language(code) lang_info = SUPPORTED_LANGUAGES[code] cx_print(t("language.changed", language=lang_info["native"]), "success") return 0 - except Exception as e: + except (ValueError, RuntimeError) as e: self._print_error(f"Failed to set language: {e}") return 1cortex/i18n/formatter.py (1)
330-351: Duration format strings are not localized.The method uses hardcoded English abbreviations ("ms", "s", "m", "h"). While this is acceptable for a CLI tool (these abbreviations are widely understood), consider adding localized duration strings to
LOCALE_CONFIGSfor full i18n consistency if needed in the future.cortex/i18n/translator.py (1)
173-180: Consider catching additional interpolation errors.The
str.format()method can raiseIndexErrorfor positional arguments andValueErrorfor malformed format strings. WhileKeyErrorhandles named arguments, broader exception handling would be safer.♻️ Proposed refinement
# Interpolate variables if kwargs: try: message = message.format(**kwargs) - except KeyError: + except (KeyError, IndexError, ValueError): # If interpolation fails, return message without interpolation pass
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
cortex/cli.pycortex/i18n/__init__.pycortex/i18n/config.pycortex/i18n/detector.pycortex/i18n/formatter.pycortex/i18n/locales/de.yamlcortex/i18n/locales/en.yamlcortex/i18n/locales/es.yamlcortex/i18n/locales/fr.yamlcortex/i18n/locales/zh.yamlcortex/i18n/translator.pydocs/i18n-guide.mdpyproject.tomltests/test_i18n.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide for Python code
Include type hints in Python code
Add docstrings for all public APIs in Python code
Use dry-run mode by default for all installation operations
Do not use silent sudo - require explicit user confirmation for privilege escalation
Implement Firejail sandboxing for execution of untrusted code
Log all operations to ~/.cortex/history.db for audit purposes
Files:
cortex/i18n/detector.pycortex/i18n/config.pycortex/i18n/__init__.pytests/test_i18n.pycortex/cli.pycortex/i18n/formatter.pycortex/i18n/translator.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain test coverage above 80% for pull requests
Files:
tests/test_i18n.py
🧬 Code graph analysis (3)
cortex/i18n/config.py (2)
cortex/i18n/detector.py (1)
detect_os_language(104-144)cortex/i18n/translator.py (5)
get_language(279-286)set_language(265-276)language(61-63)language(66-83)get_language_info(318-334)
cortex/i18n/formatter.py (3)
cortex/i18n/translator.py (3)
language(61-63)language(66-83)get_language(279-286)cortex/cli.py (1)
config(1080-1094)cortex/i18n/config.py (1)
get_language(77-107)
cortex/i18n/translator.py (2)
cortex/i18n/formatter.py (2)
language(165-167)language(170-175)cortex/i18n/config.py (3)
get_language(77-107)set_language(109-128)get_language_info(139-176)
🪛 markdownlint-cli2 (0.18.1)
docs/i18n-guide.md
188-188: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (29)
cortex/i18n/locales/zh.yaml (1)
1-307: LGTM! Comprehensive Chinese translations.The Chinese (Simplified) locale file is well-structured, mirrors the English source catalog, and preserves all placeholders for dynamic value interpolation. The translations cover all required sections for CLI i18n support.
pyproject.toml (2)
2-2: Setuptools version bump looks appropriate.The bump to
setuptools>=64.0provides better support forpackage-dataconfiguration. This version has been stable since 2022.
101-106: Package configuration correctly includes i18n module and locale data.The
cortex.i18npackage is properly added to the packages list, and thepackage-dataentry ensures YAML locale files are included in distributions. This enables runtime loading of translation catalogs.cortex/i18n/detector.py (4)
14-52: Well-structured language mappings with comprehensive variant coverage.The
SUPPORTED_LANGUAGESset andLANGUAGE_MAPPINGSdictionary properly cover regional variants for all five supported languages. The C/POSIX locale defaults to English, which follows standard POSIX behavior.
55-101: Robust locale parsing with comprehensive fallback strategies.The
_parse_localefunction handles multiple locale string formats well (e.g.,en_US.UTF-8,es_ES,zh-CN). The progressive fallback through direct mapping, language prefix, and full locale lookup ensures maximum compatibility.Type hints and docstring are present per coding guidelines.
104-144: Correct implementation of OS language detection.The function follows the standard GNU gettext priority order (
LANGUAGE→LC_ALL→LC_MESSAGES→LANG) and correctly handles the colon-separatedLANGUAGEvariable. The fallback to English ensures graceful degradation.Docstring with examples and type hints are present per coding guidelines.
147-162: Useful debugging utility for locale information.The
get_os_locale_infofunction provides comprehensive locale environment variable exposure plus the computeddetected_language, which is valuable for troubleshooting i18n issues.cortex/i18n/locales/de.yaml (1)
1-307: LGTM! Comprehensive German translations.The German locale file is well-structured, mirrors the English source catalog, and preserves all placeholders for dynamic value interpolation. The translations appear natural and cover all required CLI sections.
cortex/i18n/locales/es.yaml (1)
1-307: LGTM! Comprehensive Spanish translations.The Spanish locale file is well-structured, mirrors the English source catalog, and preserves all placeholders for dynamic value interpolation. The translations cover all required CLI sections for complete i18n support.
cortex/i18n/locales/en.yaml (1)
1-375: Well-documented source catalog with excellent translator guidance.The English source file is well-structured with:
- Clear variable documentation comments above interpolated messages (e.g., lines 35-38, 44-45)
- Consistent snake_case key naming
- Logical grouping by feature/module
- Comprehensive coverage of all CLI messages
This serves as an excellent reference for translators.
docs/i18n-guide.md (1)
1-480: Comprehensive and well-organized i18n documentation.The guide thoroughly covers all aspects of the i18n system including usage examples, adding new languages, translation guidelines, and troubleshooting. The language resolution order documentation (lines 422-429) is particularly valuable for understanding priority.
cortex/i18n/__init__.py (1)
1-58: Clean public API surface with comprehensive documentation.The module initialization properly:
- Provides a centralized entry point for all i18n functionality
- Documents usage examples in the module docstring (lines 10-20)
- Organizes
__all__exports by category for clarity- Lists all supported languages in the docstring
tests/test_i18n.py (1)
1-977: Comprehensive test suite with excellent coverage of i18n components.The test suite covers:
- Core translator functionality including language switching, interpolation, and fallback
- Configuration persistence with proper temp directory isolation
- OS language detection with various environment variable combinations
- Locale-aware formatting across all supported languages
- CLI integration for language commands
- Language name resolution for both codes and human-readable names
The use of
setUp/tearDownwithreset_translator()ensures proper test isolation. As per coding guidelines, this appears to meet the >80% coverage target for the i18n module.cortex/i18n/locales/fr.yaml (1)
1-307: French translations are complete and properly localized.The translation catalog maintains consistent structure with the English source and correctly preserves all functional
{variable}placeholders for interpolation. Verification confirms all 22 placeholders used in translations ({action},{code},{count},{current},{description},{end},{error},{id},{language},{name},{original},{package},{passed},{path},{provider},{rate},{seconds},{start},{step},{suggested},{total},{version}) match exactly between en.yaml and fr.yaml. Cultural adaptations like "NPD" for "DND" are appropriate.cortex/i18n/config.py (2)
109-128: LGTM!The
set_languagemethod properly validates input, normalizes to lowercase, and provides a clear error message with supported options.
139-176: LGTM!The
get_language_infomethod provides excellent diagnostic information with clear source attribution (environment/config/auto-detected/default), which is valuable for debugging language resolution issues.cortex/cli.py (6)
25-31: LGTM!The i18n imports are well-organized and include all necessary components for CLI internationalization.
158-161: LGTM!Good use of translation keys for user-facing API key messages.
2307-2345: LGTM!Excellent helper function that provides a user-friendly way to specify languages by code, English name, or native name. The case-insensitive matching for Latin scripts while preserving exact matching for non-Latin scripts (like 中文) is thoughtfully implemented.
2348-2386: LGTM!The
_handle_set_languagefunction provides excellent UX by listing all supported languages when an invalid input is provided, helping users discover available options.
2917-2927: LGTM!Smart handling that allows combining language setting with command execution (e.g.,
cortex --set-language es install nginx), providing a convenient one-liner workflow.
2636-2655: LGTM!The config parser setup properly mirrors the implementation with appropriate argument definitions for the language subcommand.
cortex/i18n/formatter.py (3)
14-145: LGTM!Well-researched locale configurations. The French file size units ("o", "Ko", "Mo", "Go", "To") correctly follow the French convention of using "octet" instead of "byte".
284-328: LGTM!The
format_time_agomethod correctly handles singular/plural forms and covers all common time intervals from seconds to years. The early return for negative differences (future dates) gracefully handles edge cases.
169-175: Bug: Language setter checks wrong variable.Line 172 checks
self._language(the current language) instead ofvalue(the new language being set). This means the setter always succeeds if the current language is valid, regardless of whether the new value is valid.🐛 Proposed fix
`@language.setter` def language(self, value: str) -> None: """Set the language.""" - if self._language in LOCALE_CONFIGS: + if value in LOCALE_CONFIGS: self._language = value else: self._language = "en"Likely an incorrect or invalid review comment.
cortex/i18n/translator.py (4)
95-116: LGTM!Good defensive implementation. Using
yaml.safe_loadprotects against arbitrary code execution, and the silent fallback for missing catalogs ensures graceful degradation.
247-262: LGTM!Clean lazy initialization with appropriate use of
LanguageConfigfor language resolution. The debug mode viaCORTEX_I18N_DEBUGenvironment variable is valuable for translation development and testing.
289-309: LGTM!The
t()function provides a clean, concise API for translations throughout the codebase. The docstring examples clearly demonstrate usage patterns.
337-344: LGTM!Returning a copy of
SUPPORTED_LANGUAGESis good defensive programming, preventing accidental mutation of the module-level constant.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@coderabbitai Review. |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
@RIVALHIDE Added some comments.
Also, No test for malformed YAML handling. There are still some coderabbit comments left to address.
…f truth for languages, named time constants, non-Latin script handling, and malformed YAML resilience
CLA Verification PassedAll contributors have signed the CLA.
|
|
@RIVALHIDE will it be able to handle translation for any new command that is added to the Cortex in the future ? |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@cortex/cli.py`:
- Around line 1503-1530: The output for `config language` leaks untranslated
English: replace raw source keys and hardcoded hint strings in the `--info`
printing block and the "no code" branch with the existing translation calls used
by `_config_show` and other `config.*` keys; specifically, use the `t(...)`
function for labels like environment/source/detected_language (instead of raw
"environment") and swap the hardcoded hints shown in the no-code branch (the two
console.print lines that start with "[dim]Use: cortex config language...") to
call the appropriate `t('config.*')` keys (or reuse the same keys `_config_show`
uses) and output via `cx_print`/`console.print` so all user-facing text in
`get_language`, `SUPPORTED_LANGUAGES` display, and the `--info` branch is
localized.
- Around line 2972-3043: The lookup fails for accented Latin names because
_normalize_for_lookup and _resolve_language_name use .lower(); change
normalization to use .casefold() for ASCII/Latin handling so accented inputs
like "español" or "français" match; update _normalize_for_lookup to return
s.casefold() when _is_ascii(s) (or otherwise when ASCII/Latin detection applies)
and then in _resolve_language_name build keys with english_name.casefold(), use
native_normalized = _normalize_for_lookup(native_name) and still preserve the
original native_name in name_to_code for exact matches; ensure
SUPPORTED_LANGUAGES checks use the same normalization logic.
In `@cortex/i18n/detector.py`:
- Around line 95-121: The locale parsing in the detector (working on
locale_string and LANGUAGE_MAPPINGS) only handles underscores and ignores
hyphenated locales like "en-US"; update the normalization so that hyphens are
converted to underscores (e.g., replace "-" with "_") immediately after
lowercasing/stripping and before removing encoding suffix and performing
lookups, then use the normalized full_locale and language part (from split("_"))
when checking LANGUAGE_MAPPINGS in the existing branches (direct mapping,
language part, regional variant).
In `@cortex/i18n/locales/en.yaml`:
- Around line 120-128: The file contains duplicate top-level keys "cache" and
"env" so later blocks overwrite earlier translations and drop keys like
cache.saved_calls and cache.read_error and many env.* labels; locate all "cache"
and "env" blocks (e.g., the cache block with keys stats_header, hits, misses,
hit_rate, saved_calls, read_error, unexpected_error and the various env.*
labels) and merge their entries into a single "cache" map and a single "env" map
respectively, removing the duplicate blocks and ensuring no keys are lost
(combine values, preferring the most complete/accurate strings for any
conflicting keys). Ensure the merged sections include all previously defined
keys such as cache.saved_calls, cache.read_error, unexpected_error, and all
env.* labels so YAML retains every translation.
In `@cortex/i18n/locales/zh.yaml`:
- Around line 32-44: The YAML defines the top-level mapping key "language" twice
which causes the first block (keys like changed, current, auto_detected,
set_from_env, set_from_config, default, available, invalid_code,
supported_codes, select_prompt, switched_to) to be silently overwritten; merge
the missing key(s) such as set_failed into the first "language" mapping and
remove the duplicate second "language" block so all translations live under a
single "language" mapping (ensure keys like changed, switched_to and set_failed
are present and deduplicated).
In `@tests/test_i18n.py`:
- Around line 854-868: The test method test_cli_language_list contains an unused
import "sys"; remove that import to clean up the test. Edit the
test_cli_language_list function and delete the line importing sys so only the
required imports (patch, argparse, CortexCLI) remain, leaving the rest of the
test (creating argparse.Namespace and calling CortexCLI().config) unchanged.
🧹 Nitpick comments (2)
cortex/i18n/config.py (1)
247-283: Add audit logging for language preference changes.Language updates are persisted but not recorded in the history DB. If audit logging is required for all operations, please record set/clear events (e.g., via
InstallationHistorywith a CONFIG-like type). As per coding guidelines, please log operations to~/.cortex/history.db.tests/test_i18n.py (1)
759-769: Consider usingimportlib.resourcesfor more robust path resolution.The current path construction assumes a specific directory structure relative to the test file. While this works for standard test execution, it could break if the project structure changes.
♻️ Optional: Use importlib.resources for path resolution
def test_all_supported_languages_have_catalogs(self): """Test that all supported languages have message catalogs.""" - from pathlib import Path - + import importlib.resources from cortex.i18n.translator import SUPPORTED_LANGUAGES - locales_dir = Path(__file__).parent.parent / "cortex" / "i18n" / "locales" - for lang_code in SUPPORTED_LANGUAGES: - catalog_path = locales_dir / f"{lang_code}.yaml" - self.assertTrue(catalog_path.exists(), f"Missing catalog for language: {lang_code}") + # Check if resource is accessible within the package + try: + with importlib.resources.files("cortex.i18n.locales").joinpath(f"{lang_code}.yaml").open(): + pass + except FileNotFoundError: + self.fail(f"Missing catalog for language: {lang_code}")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cortex/cli.pycortex/i18n/config.pycortex/i18n/detector.pycortex/i18n/formatter.pycortex/i18n/locales/en.yamlcortex/i18n/locales/zh.yamltests/test_i18n.py
🚧 Files skipped from review as they are similar to previous changes (1)
- cortex/i18n/formatter.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide for Python code
Include type hints in Python code
Add docstrings for all public APIs in Python code
Use dry-run mode by default for all installation operations
Do not use silent sudo - require explicit user confirmation for privilege escalation
Implement Firejail sandboxing for execution of untrusted code
Log all operations to ~/.cortex/history.db for audit purposes
Files:
cortex/cli.pytests/test_i18n.pycortex/i18n/config.pycortex/i18n/detector.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain test coverage above 80% for pull requests
Files:
tests/test_i18n.py
🧬 Code graph analysis (2)
tests/test_i18n.py (4)
cortex/i18n/formatter.py (13)
language(181-183)language(186-191)LocaleFormatter(164-367)format_date(197-208)format_date(400-402)format_number(238-265)format_number(415-417)format_file_size(267-298)format_file_size(420-422)format_datetime(223-236)format_datetime(410-412)format_time(210-221)format_time(405-407)cortex/i18n/config.py (3)
set_language(247-273)get_language(211-245)LanguageConfig(50-328)cortex/cli.py (5)
config(1469-1483)CortexCLI(62-2969)_resolve_language_name(2997-3053)_handle_set_language(3056-3094)main(3158-3997)cortex/i18n/detector.py (2)
detect_os_language(124-164)get_os_locale_info(167-182)
cortex/i18n/config.py (2)
cortex/i18n/detector.py (1)
detect_os_language(124-164)cortex/i18n/translator.py (5)
t(291-311)get_language(281-288)set_language(267-278)language(61-63)language(66-83)
🪛 YAMLlint (1.37.1)
cortex/i18n/locales/en.yaml
[error] 431-431: duplication of key "env" in mapping
(key-duplicates)
[error] 459-459: duplication of key "cache" in mapping
(key-duplicates)
cortex/i18n/locales/zh.yaml
[error] 131-131: duplication of key "language" in mapping
(key-duplicates)
🔇 Additional comments (9)
tests/test_i18n.py (9)
1-19: LGTM!The module docstring clearly documents the test coverage areas, and the imports are appropriate for the test suite.
21-212: LGTM!The
TestTranslatorclass provides comprehensive coverage of the translator functionality including language switching, variable interpolation, debug mode, singleton behavior, and translations across all 5 supported languages. The setUp/tearDown methods properly reset the global translator state for test isolation.
235-346: LGTM!Excellent coverage of malformed YAML edge cases. These tests validate the production hardening mentioned in the commit messages, ensuring the config loader is resilient to:
- Malformed YAML syntax
- Empty files
- Whitespace-only files
- Invalid root types (list, string instead of dict)
- Non-string language values
- Null language values
This defensive testing prevents crashes in production when users have corrupted config files.
423-528: LGTM!The environment variable precedence tests correctly verify the documented priority order:
LANGUAGE→LC_ALL→LC_MESSAGES→LANG. The fallback scenarios (unsupported languages, C/POSIX locales, empty environment) are well covered.
530-754: LGTM!The
TestLocaleFormatterclass provides solid coverage of locale-aware formatting including dates, numbers, file sizes, relative times, and durations across multiple locales. The invalid language fallback test (lines 748-753) ensures robustness.
788-832: LGTM!The
TestTranslationCompletenessclass directly addresses the PR objective of ensuring "complete message translation with graceful fallback to English (avoid English leaks)". The tests verify that:
- Common keys are translated in all languages
- Spanish translations aren't just English placeholders
- Variable interpolation works correctly across all languages
964-1037: LGTM!The
TestResolveLanguageNameclass provides excellent coverage of the language resolution logic, including:
- Direct code matches
- English name resolution (case-insensitive)
- Native name resolution (including non-Latin scripts like 中文)
- Whitespace handling
- Invalid/unsupported language rejection
The tests for non-Latin script handling (lines 1013-1029) directly validate the PR's handling of scripts where
.lower()is meaningless.
1039-1102: LGTM!The
TestSetLanguageFlagclass validates the--set-languageglobal CLI flag, ensuring it correctly handles language codes, English names, native names, and invalid input with appropriate exit codes.
1-1105: Multi-process file locking is already properly implemented incortex/i18n/config.py.The
LanguageConfigclass uses boththreading.Lockfor intra-process synchronization (line 73) andfcntl.flockfor inter-process synchronization (lines 96-118), with platform-specific handling for Windows. The implementation includes exclusive locks for writes and shared locks for reads, with comprehensive error handling and documentation. No additional concurrency tests are needed intest_i18n.pyas the codebase has a dedicatedtest_thread_safety.pyfor concurrent access scenarios.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@Suyashd999 Yes, the system supports new commands, but developers must manually add translation keys to en.yaml and other locale files — there's no auto-translation. |
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@RIVALHIDE Kindly address bot comments and then create a new Issue regarding auto-translation thing.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cortex/cli.py`:
- Around line 3085-3123: The hardcoded header "[bold]Supported
languages:[/bold]" in _handle_set_language causes an English string leak;
replace it with a call to the i18n translator (use the existing t() helper) e.g.
console.print(t("language.supported_languages_header")) preserving formatting
(bold) as needed, and add the corresponding translation key/value in the locale
files so all supported locales show a localized header; update any tests or docs
that expect the old literal if present.
In `@cortex/i18n/detector.py`:
- Around line 75-123: _parse_locale currently strips encoding suffixes but
leaves locale modifiers like "@latin", causing wrong fallbacks; modify the
normalization to also remove any "@modifier" suffix (and ensure this occurs
after hyphen->underscore normalization and encoding removal) so entries like
"en_US.UTF-8@latin" or "sr_RS@latin" reduce to the canonical "en_us" / "sr_rs"
before checking LANGUAGE_MAPPINGS; update the regex used to strip suffixes in
_parse_locale (or add a second strip step) to remove both the ".encoding" and
any "@modifier" portions prior to the LANGUAGE_MAPPINGS lookups.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cortex/cli.pycortex/i18n/config.pycortex/i18n/detector.pycortex/i18n/locales/en.yamlcortex/i18n/locales/zh.yamltests/test_i18n.py
🚧 Files skipped from review as they are similar to previous changes (2)
- cortex/i18n/locales/en.yaml
- cortex/i18n/locales/zh.yaml
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide for Python code
Include type hints in Python code
Add docstrings for all public APIs in Python code
Use dry-run mode by default for all installation operations
Do not use silent sudo - require explicit user confirmation for privilege escalation
Implement Firejail sandboxing for execution of untrusted code
Log all operations to ~/.cortex/history.db for audit purposes
Files:
cortex/i18n/detector.pycortex/i18n/config.pycortex/cli.pytests/test_i18n.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain test coverage above 80% for pull requests
Files:
tests/test_i18n.py
🧬 Code graph analysis (1)
cortex/cli.py (3)
cortex/i18n/config.py (5)
LanguageConfig(50-374)get_language(211-245)set_language(247-277)get_language_info(291-335)clear_language(279-289)cortex/i18n/translator.py (6)
get_language(281-288)set_language(267-278)t(291-311)get_language_info(320-336)language(61-63)language(66-83)cortex/branding.py (2)
cx_print(62-82)cx_header(95-101)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Test (Python 3.11)
- GitHub Check: Test (Python 3.12)
- GitHub Check: Test (Python 3.10)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
🔇 Additional comments (18)
cortex/i18n/detector.py (3)
21-34: Single-source supported languages is clean.
Nice to derive codes directly fromtranslator.SUPPORTED_LANGUAGESto avoid drift.
126-166: Detection order and supported-code filtering look solid.
Priority and validation are clear and consistent.
169-184: Debug locale info helper is clear and useful.cortex/i18n/config.py (4)
35-47: Supported-language derivation is consistent.
69-210: Concurrency + atomic write handling is robust.
Thread locks, file locks, and temp-file rename make persistence resilient.
211-335: Resolution order and info payload are clear.
Good separation of env/config/auto/default sources.
337-374: Audit logging of language changes is a good addition.tests/test_i18n.py (3)
21-848: Broad i18n coverage looks solid.
Great breadth across translator, config, detector, formatter, and catalog integrity.
849-1117: CLI integration and name-resolution tests are solid.
582-589: This review comment is incorrect: the code does not use Babel for number formatting.The
LocaleFormatter.format_number()method uses hardcoded locale separators from theLOCALE_CONFIGSdictionary, where French is configured with a plain space (U+0020). The test assertion is correct and not brittle—the formatter will consistently return"1 234 567"with regular spaces because the separator is fixed in the configuration, not determined by an external library like Babel.The proposed regex fix is unnecessary and misleading, as it would incorrectly suggest the function can return multiple separator types when it cannot.
Likely an incorrect or invalid review comment.
cortex/cli.py (8)
170-172: Localized API-key missing prompts look good.
493-566: Stack list/install flows are properly localized.
581-598: Sandbox usage header/intro now localized.
858-973: Install progress/status messaging is now i18n-friendly.Also applies to: 1008-1010
1452-1463: Cache stats output is localized consistently.
1469-1616: Config language output is well localized and consistent.
2984-3083: Language-name normalization/resolution logic looks robust.
3241-3247: Parser wiring for--set-languageandconfig languagelooks solid.Also applies to: 3471-3491, 3874-3885, 3961-3963
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@cortex/i18n/locales/de.yaml`:
- Around line 1-309: The de.yaml locale is missing 79 keys causing mixed
English/German UI; add the entirely missing sections (ui, docker, role) and add
the missing keys in existing sections (env, config, sandbox, cache, errors,
wizard) so their keys and placeholder tokens exactly match en.yaml; for each
added key provide a German translation preserving placeholders like
{count}/{name}/{error}, then run the existing locale validation (the same
schema/validation used to compare en.yaml) to ensure no extra/mismatched keys
remain and confirm the i18n loader uses the updated de locale for German users.
In `@cortex/i18n/locales/es.yaml`:
- Around line 1-309: The Spanish locale is missing entire sections and keys (add
the missing top-level sections "ui", "docker", and "role" plus any extended keys
present in en.yaml that are absent here) so update the es.yaml to mirror
en.yaml's structure: copy the missing "ui", "docker", and "role" sections and
any additional keys that de.yaml lacked, provide Spanish translations ensuring
placeholders (e.g., {name}, {id}, {count}, {error}, {package}, {language},
{code}) exactly match the English keys, and keep existing keys' placeholder
formats unchanged so fallback behavior is not relied upon.
In `@cortex/i18n/locales/fr.yaml`:
- Around line 1-309: The French locale is missing the same keys present in
de.yaml and es.yaml; compare fr.yaml's top-level sections (common, language,
cli, config, install, stack, sandbox, history, rollback, doctor, wizard, notify,
env, api_key, cache, errors, progress) against the other locales and add any
missing entries (including pluralization variants, placeholdered strings like
{count},{id},{error},{package},{version},{step},{total}, and keys under
cli.commands and cli.args) so fr.yaml structurally matches de.yaml/es.yaml;
preserve existing French wording (including "NPD" in notify.dnd_updated) and
ensure all placeholder names and key names match exactly.
🧹 Nitpick comments (6)
cortex/i18n/detector.py (1)
75-130: Remove redundant lookup at lines 126-128.The check at lines 126-128 is unreachable dead code. If
locale_lowermatchedLANGUAGE_MAPPINGS, it would have already returned at line 117-118. This branch can never execute sincelocale_lowerhasn't changed between line 117 and line 127.♻️ Proposed fix
# Try just the language part (before underscore) if "_" in locale_lower: lang_part = locale_lower.split("_")[0] if lang_part in LANGUAGE_MAPPINGS: return LANGUAGE_MAPPINGS[lang_part] - - # Try full locale for regional variants (already normalized) - if locale_lower in LANGUAGE_MAPPINGS: - return LANGUAGE_MAPPINGS[locale_lower] return Nonecortex/cli.py (5)
1547-1548: Duplicated import ofreset_translatorwithin same method.The
reset_translatorfunction is imported twice within_config_language()at lines 1547-1548 and 1568-1570. Move the import to the top of the method or to the module level to avoid redundancy.♻️ Proposed fix
def _config_language(self, args: argparse.Namespace) -> int: """Handle language configuration.""" + from cortex.i18n.translator import reset_translator + lang_config = LanguageConfig() # ... (list and info handling) ... # Handle 'auto' to clear saved preference if code.lower() == "auto": lang_config.clear_language() - from cortex.i18n.translator import reset_translator - reset_translator() # ... # Validate and set language # ... try: lang_config.set_language(code) # Reset the global translator to pick up the new language - from cortex.i18n.translator import reset_translator - reset_translator() set_language(code)Also applies to: 1568-1570
1506-1515: Duplicated source translation mapping.The
source_translation_keys/source_translationsdictionary is defined identically in both_config_language()(lines 1506-1515) and_config_show()(lines 1593-1599). Extract this to a module-level constant or helper function to follow DRY principles.♻️ Proposed fix
# At module level or within the class _SOURCE_TRANSLATION_KEYS = { "environment": "language.set_from_env", "config": "language.set_from_config", "auto-detected": "language.auto_detected", "default": "language.default", } def _translate_source(source: str) -> str: """Translate a language source identifier to localized text.""" key = _SOURCE_TRANSLATION_KEYS.get(source) return t(key) if key else sourceAlso applies to: 1593-1599
1611-1612: Hardcoded configuration paths in display.The paths
~/.cortex/preferences.yamland~/.cortex/history.dbare hardcoded. Consider using the same path resolution logic used elsewhere (e.g., fromLanguageConfigorInstallationHistory) to ensure consistency if these paths ever change.
3012-3022: Consider movingunicodedataimport to module level.The
import unicodedatastatement inside_normalize_for_lookup()is executed on every call. While Python caches imports, moving it to the module level is cleaner and follows the project's import style.♻️ Proposed fix
+import unicodedata import argparse import logging # ... other imports ... def _normalize_for_lookup(s: str) -> str: # ... try: - import unicodedata - has_latin = any(unicodedata.category(c).startswith("L") and ord(c) < 0x3000 for c in s)
1565-1575: Consider audit logging for language preference changes.Per coding guidelines, operations should be logged to
~/.cortex/history.dbfor audit purposes. Language configuration changes are not currently logged. While this is a minor configuration change rather than an installation, consider adding a log entry for traceability if strict audit compliance is required.Based on coding guidelines: "Log all operations to ~/.cortex/history.db for audit purposes"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cortex/cli.pycortex/i18n/detector.pycortex/i18n/locales/de.yamlcortex/i18n/locales/en.yamlcortex/i18n/locales/es.yamlcortex/i18n/locales/fr.yamlcortex/i18n/locales/zh.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- cortex/i18n/locales/zh.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide for Python code
Include type hints in Python code
Add docstrings for all public APIs in Python code
Use dry-run mode by default for all installation operations
Do not use silent sudo - require explicit user confirmation for privilege escalation
Implement Firejail sandboxing for execution of untrusted code
Log all operations to ~/.cortex/history.db for audit purposes
Files:
cortex/cli.pycortex/i18n/detector.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Test (Python 3.11)
- GitHub Check: Test (Python 3.12)
- GitHub Check: Test (Python 3.10)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
🔇 Additional comments (18)
cortex/i18n/detector.py (5)
1-18: LGTM! Module docstring and imports are appropriate.The module docstring clearly documents the purpose and the environment variables checked. Type hints are properly set up with
TYPE_CHECKINGimport pattern.
21-34: LGTM! Clean implementation with proper circular import handling.The late import pattern and docstring are well done. The function correctly derives supported codes from the single source of truth.
37-72: LGTM! Comprehensive locale variant mappings.The mapping covers common regional variants for all supported languages and correctly handles C/POSIX locales.
133-173: LGTM! Correct environment variable priority and handling.The function properly implements GNU gettext-style language detection with correct priority ordering and handles the colon-separated LANGUAGE variable appropriately.
176-191: LGTM! Useful diagnostic function with proper typing.The function provides comprehensive locale information for debugging purposes. The type hint correctly reflects that environment variable values may be
None.cortex/i18n/locales/en.yaml (2)
1-471: LGTM! English source catalog is well-structured.The file is properly organized with clear section headers, appropriate placeholder documentation via comments, and no duplicate top-level keys. The previous issue with duplicate
envandcachesections appears to have been resolved.The comprehensive key coverage in en.yaml serves as the authoritative source for fallback when translations are missing in other locales.
35-38: Good practice: Placeholder documentation for translators.The inline comments documenting placeholder variables (e.g.,
{action},{error}) provide clear guidance for translators. This pattern is consistently applied throughout the file.cortex/cli.py (11)
25-31: LGTM - i18n imports are well-structured.The imports from
cortex.i18nprovide all necessary utilities for internationalization support throughout the CLI.
170-172: LGTM - API key messages properly internationalized.User-facing error messages and hints are correctly routed through the translation function.
496-566: LGTM - Stack command messages properly internationalized.The stack-related user messages use translation keys consistently with proper parameter interpolation for dynamic values like stack names and package counts.
582-598: Sandbox command descriptions remain hardcoded.The sandbox usage section translates headers but keeps command syntax descriptions in English (lines 585-591). This is likely intentional for CLI command consistency, but verify if these should be translated for full i18n coverage.
859-1009: LGTM - Installation flow comprehensively internationalized.The install command's user-facing messages (analysis, planning, execution, success/failure) are properly routed through translation with appropriate parameter interpolation.
1452-1462: LGTM - Cache statistics display internationalized.Cache stats messages and error handling use translation keys with proper error parameter interpolation.
3085-3124: LGTM -_handle_set_languageimplementation is correct.The function properly resolves language names, validates against
SUPPORTED_LANGUAGES, persists the preference, resets the translator, and provides user feedback with appropriate error handling.
3241-3247: LGTM - Global language flag well-defined.The
--set-languageflag accepts both language codes and human-readable names, with clear help text showing examples.
3471-3490: LGTM - Config command parser structure is correct.The
config languagesubcommand properly supports setting by code, listing available languages, and showing current configuration info.
3874-3884: LGTM - Global language flag handling is well-implemented.The logic correctly handles three scenarios:
- Language-only command (
cortex --set-language es) - sets language and exits- Language setting fails - returns error immediately
- Language + command (
cortex --set-language es install nginx) - sets language then continuesThe inline comment clearly documents this behavior.
3961-3962: LGTM - Config command routing correctly added.The config command is properly routed to the
cli.config(args)method.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
mikejmorgan-ai
left a comment
mikejmorgan-ai
left a comment
There was a problem hiding this comment.
Review: APPROVED ✅
i18n Support - 5 Languages (PR #593)
Code Quality
- ✅ Thread-safe file operations with proper locking (threading.Lock + fcntl.flock)
- ✅ TOCTOU race condition properly addressed - exists() check and file read within critical section
- ✅ Formatter singleton correctly syncs language on each access
- ✅ Comprehensive error handling for malformed YAML, empty files, invalid types
- ✅ Well-documented code with clear docstrings
Test Coverage
- ✅ 89 tests all passing
- ✅ Tests cover: translator, config, detector, formatter, CLI integration
- ✅ Edge cases tested: malformed YAML, null values, invalid types, whitespace files
- ✅ All 5 languages verified: English, Spanish, French, German, Chinese
Architecture
- ✅ Clean separation: translator.py, formatter.py, config.py, detector.py
- ✅ Single source of truth for supported languages (SUPPORTED_LANGUAGES dict)
- ✅ Proper fallback chain: env var → config file → OS detection → default
Notes for CI
- Requires Python 3.10+ due to union type syntax (
type | None) - Ensure CI runs with compatible Python version
Ready for merge once Ansh approves.
Reviewed-by: Claude Code
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cortex/cli.py (1)
516-540: Require explicit--executefor stack installs (dry‑run by default).
cortex stack <name>currently performs a real install unless--dry-runis passed. This conflicts with the dry‑run‑by‑default safety requirement for installation operations in this CLI.🔧 Suggested fix
@@ - if args.dry_run: - return self._handle_stack_dry_run(stack, packages) - - return self._handle_stack_real_install(stack, packages) + execute = getattr(args, "execute", False) + if args.dry_run or not execute: + return self._handle_stack_dry_run(stack, packages) + + return self._handle_stack_real_install(stack, packages)@@ stack_parser.add_argument( "--dry-run", action="store_true", help="Show what would be installed (requires stack name)" ) +stack_parser.add_argument( + "--execute", + action="store_true", + help="Execute stack installation (default: dry-run)", +)As per coding guidelines, please make dry‑run the default for stack installs.
🤖 Fix all issues with AI agents
In `@cortex/cli.py`:
- Around line 581-592: The sandbox command help prints per-command descriptions
in English; replace each hardcoded description in the no-action help block (the
strings currently printed in the if not action branch of cortex/cli.py) with
translated keys via the t(...) helper (e.g. t("sandbox.cmd_create"),
t("sandbox.cmd_install"), t("sandbox.cmd_test"), t("sandbox.cmd_promote"),
t("sandbox.cmd_cleanup"), t("sandbox.cmd_list"), t("sandbox.cmd_exec")) while
preserving the command syntax and spacing, and add corresponding sandbox.cmd_*
entries to the locale files so translations are available.
🧹 Nitpick comments (1)
cortex/cli.py (1)
1525-1563: Allowconfig languageto accept human‑readable names.
--set-languagealready resolves names via_resolve_language_name, butconfig languageonly accepts codes. Reusing the resolver keeps UX consistent.♻️ Recommended refactor
@@ - # Validate and set language - code = code.lower() - if code not in SUPPORTED_LANGUAGES: - self._print_error(t("language.invalid_code", code=code)) + # Validate and set language (allow names as well as codes) + code_input = code + resolved = _resolve_language_name(code_input) + if not resolved: + self._print_error(t("language.invalid_code", code=code_input)) console.print( f"[dim]{t('language.supported_codes')}: {', '.join(SUPPORTED_LANGUAGES.keys())}[/dim]" ) return 1 + code = resolved
…complete locale parity
…tions to avoid English leaks.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cortex/cli.py (1)
699-709: Updatesandbox_promoteto default to dry-run mode.The
--dry-runflag usesaction="store_true"(line 3522) without a default value, requiring users to explicitly opt-in. Per coding guidelines, all installation operations must default to dry-run. The current implementation allowsdry_runto default toFalse(line 703), which contradicts the requirement to "implement dry-run by default for all installation operations." This should align with other protective operations likeremove.
🤖 Fix all issues with AI agents
In `@cortex/cli.py`:
- Around line 500-501: Replace the hardcoded English fallback strings in the
stack printing calls so they use the app’s localization keys instead of literal
text: change stack.get('name', 'Unnamed Stack') and stack.get('description', 'No
description') to use the translator lookup (e.g., t("stack.unnamed") and
t("stack.no_description") or the project's gettext wrapper) as the default
values; also add the corresponding keys ("stack.unnamed" and
"stack.no_description") to the locale files so the defaults are localized.
🧹 Nitpick comments (2)
cortex/cli.py (2)
1593-1599: Duplicate source translation mapping - consider extracting.The
source_translationsdictionary is duplicated here and in_config_language(lines 1506-1511). This violates DRY and could lead to inconsistencies if keys change.♻️ Proposed refactor
Extract to a module-level constant or helper function:
# Near the top of the file or in a dedicated section def _get_source_display(source: str) -> str: """Translate language source identifier to localized display string.""" source_keys = { "environment": "language.set_from_env", "config": "language.set_from_config", "auto-detected": "language.auto_detected", "default": "language.default", } key = source_keys.get(source) return t(key) if key else sourceThen use in both methods:
- source_translations = { - "environment": t("language.set_from_env"), - ... - } - source_display = source_translations.get(lang_info["source"], lang_info["source"]) + source_display = _get_source_display(lang_info["source"])
3012-3021: Consider movingunicodedataimport to module level.The
unicodedataimport inside_normalize_for_lookupadds overhead on each call. While negligible for the current 5-language set, this could be moved to the module level for cleaner code sinceunicodedatais a standard library module with no startup cost concerns.♻️ Proposed refactor
import argparse import logging import os import sys import time +import unicodedata import uuidThen remove the inline import:
try: - import unicodedata - has_latin = any(unicodedata.category(c).startswith("L") and ord(c) < 0x3000 for c in s)
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@cortex/cli.py`:
- Around line 906-907: The printed executing message appends an extra "..."
while the translation key t('install.executing') already contains ellipses;
update the print call in cortex/cli.py that currently reads
print(f"\n{t('install.executing')}...") to remove the added ellipses so it only
prints print(f"\n{t('install.executing')}"), preserving the translation string
as the sole source of punctuation.
- Around line 1452-1457: The printed cache hit-rate is using the translation
string t('cache.hit_rate') without substituting its {rate} placeholder and
saved_calls is incorrectly using stats.hits; update the three cx_print calls in
this block: format t('cache.hit_rate') with .format(rate=hit_rate) (or
equivalent interpolation) when calling cx_print for the hit rate, and use
stats.saved_calls (not stats.hits) for the saved calls line so the labels and
values are correct; keep using cx_print and t(...) as the surrounding helpers.
- Around line 170-172: The error output is using a hardcoded "Error:" prefix so
localized messages still get an English prefix; update the printing to use the
i18n label instead of the hardcoded string by changing the call path that prints
errors (the _print_error helper or its callers) to prepend t("error.prefix") (or
the appropriate i18n key used elsewhere) rather than the literal "Error:";
locate and modify the _print_error implementation or replace its usage around
the lines where _print_error(t("api_key.not_found")) is called so the prefix
comes from t(...) and ensure cx_print/t usage remains consistent for info hints.
|
4 participants
Related Issue
Closes #93
Summary
Adds comprehensive multi-language support (i18n) to the Cortex CLI with proper localization, formatting, tests, and documentation.
Supported Languages
enesfrdezhKey Features
cortex config language <code>orcortex --set-language "English"LANG,LC_ALL,LANGUAGEenvironment variables~/.cortex/preferences.yamlCORTEX_I18N_DEBUG=1) to show translation keysUsage Examples
Set language by code
cortex config language es
Set language by name (human-readable)
cortex --set-language "Spanish"
View current language info
cortex config language --info
List available languages
cortex config language --list
Use auto-detection
cortex config language --auto
Clear preference (use OS default)
cortex config language --clear
Files Changed
pyproject.tomlcortex.i18npackage, locale YAML files as package data, upgraded setuptools to >=64.0 for editable install supportMANIFEST.inrecursive-include cortex/i18n/locales *.yamlto include translations in PyPI distributioncortex/cli.pyt()calls, addedconfig languagesubcommand, added--set-languageglobal flagcortex/i18n/__init__.pyt,set_language,get_language, etc.)cortex/i18n/translator.pycortex/i18n/config.py~/.cortex/preferences.yaml, preference resolution ordercortex/i18n/detector.pyLANG,LC_ALL,LANGUAGE,LC_MESSAGES)cortex/i18n/formatter.pycortex/i18n/locales/en.yamlcortex/i18n/locales/es.yamlcortex/i18n/locales/fr.yamlcortex/i18n/locales/de.yamlcortex/i18n/locales/zh.yamltests/test_i18n.pydocs/i18n-guide.mdAI Disclosure
Used Cursor AI (Claude opus 4.5) to assist with:
Checklist
type(scope): descriptionor[scope] descriptionpytest tests/)Architecture
Language Resolution Order
CORTEX_LANGUAGEenvironment variable (highest priority)~/.cortex/preferences.yamlLANG/LC_ALLDemo video
Screencast.from.14-01-26.07.13.17.PM.IST.webm
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.